Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve popup position #10257

Merged
merged 2 commits into from
Apr 20, 2024

Conversation

karthago1
Copy link
Contributor

@karthago1 karthago1 commented Apr 7, 2024

Make the popup positions more consistent.
Improvements:

  1. if the signature popup content is bigger than the available space, then the popup is always shown under the cursor, even if there more space above the cursor than below. check this video
  2. There is no mutation anymore inside required_size. Maybe in the future we can update all widgets to have no mutations and change the trait

the following is the new behavior
asciicast

IMO this make at least the completion dialog consistent and will be shown always bellow the cursor.

Edit: Video Updated

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks pretty promising. I left some comments but I didn't have time to read trough everything I detail yet.

helix-term/src/ui/popup.rs Outdated Show resolved Hide resolved
helix-term/src/ui/popup.rs Show resolved Hide resolved
helix-term/src/ui/menu.rs Outdated Show resolved Hide resolved
helix-term/src/ui/popup.rs Outdated Show resolved Hide resolved
helix-term/src/ui/popup.rs Show resolved Hide resolved
@karthago1 karthago1 marked this pull request as ready for review April 8, 2024 17:20
@pascalkuthe
Copy link
Member

Judging from your video there is either an existing bug or something new introduced with this PR going on. The signature help should be hidden automatically if it interests with the completions. In stead case the completion doc popup seem to overlap.

@pascalkuthe pascalkuthe added A-helix-term Area: Helix term improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 9, 2024
@karthago1
Copy link
Contributor Author

karthago1 commented Apr 9, 2024

yes I saw that, I will check it and try to fix it

@karthago1 karthago1 force-pushed the improve-popup-position branch 2 times, most recently from 969d591 to f37c35b Compare April 9, 2024 20:18
@karthago1 karthago1 marked this pull request as draft April 9, 2024 20:19
@karthago1 karthago1 force-pushed the improve-popup-position branch 3 times, most recently from 4f8044a to 38b7815 Compare April 11, 2024 19:56
@karthago1
Copy link
Contributor Author

Until now the SignatureHelp Popup is removed and triggered in the Completion logic. I removed this logic and instead the completion view store it's used areas in the Editor. Later when rendering the SignatureHelp. the completion areas (from the editor) are checked and if they intersect, then it is not rendered.

the current solution is more responsive when comes to hide/show the popups.

(I have update the video in the PR description)

@karthago1 karthago1 marked this pull request as ready for review April 11, 2024 20:17
@pascalkuthe
Copy link
Member

pascalkuthe commented Apr 11, 2024

Until now the SignatureHelp Popup is removed and triggered in the Completion logic. I removed this logic and instead the completion view store it's used areas in the Editor. Later when rendering the SignatureHelp. the completion areas (from the editor) are checked and if they intersect, then it is not rendered.

the current solution is more responsive when comes to hide/show the popups.

(I have update the video in the PR description)

hmm I don't love that. I get why you implemented it that way, there is no other way to share this information and it may change at any point due to resizing. I am not a fan of that tough. I want to really stop adding ad-hoc fields to Editor. I also want to move the component infrastructure to be a more typical passive/immutable widget tree with no mutation inside the rendering (except maybe for some caches but that can be interior mutability).

For example one possibility could be that we use either somekind of popup container that manages all open popus to hide overlapping popups (or do it in the compositor directly that's really where that kind of thing belongs IMO).

The first step for that is turning the completions menu into a proper component instead of the spaghetti code we currently have (that is a historical artifact that I think is not necessary anymore now with the event system). I am already working on that in a much larger PR that makes manly changes to the completion menu (the refactor there started for unrelated reasons but I see now that its really the right change anyway)

There could also be other approaches I have some ideas but I think all of them will involve larger changes. I don't think you need to tackle that here. Ther rest of the PR is a nice not too large change so I would prefer to just revert the changes you made to the signature help popup and rather merge this as is. Sorry for not thinking about that earlier when I made that comment I thaught that was maybe a new regression with this PR.

@karthago1
Copy link
Contributor Author

karthago1 commented Apr 12, 2024

I reverted as you asked.

I want to really stop adding ad-hoc fields to Editor.

we can put the field directly under Context. Context is usually used for bad stuff 😄

For example one possibility could be that we use either somekind of popup container that manages all open popus to hide overlapping popups (or do it in the compositor directly that's really where that kind of thing belongs IMO).

I thought about a PopupManager but then the adhoc was too attractive and easy to implement and IMO it leaves the code base in better state, since it remove any dependency from the Completion to Signature Popup.

The first step for that is turning the completions menu into a proper component instead of the spaghetti code we currently have (that is a historical artifact that I think is not necessary anymore now with the event system). I am already working on that in a much larger PR that makes manly changes to the completion menu (the refactor there started for unrelated reasons but I see now that its really the right change anyway)

nice then I will wait for your PR first before making further changes

when I made that comment I thaught that was maybe a new regression with this PR.

yes it is not. Th regression appears only if there no space except in top, and the window must be very small. So I think this PR is still improving the popup positions much. (In my screen I never encounter it.)

I am thinking of creating a new PR to add some key binding to toggle on/off the Signature Popup. Sometimes it is annoying. DO you know if it is possible to toggle off the Signature popup in helix?

pascalkuthe
pascalkuthe previously approved these changes Apr 14, 2024
@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 14, 2024
@karthago1
Copy link
Contributor Author

@pascalkuthe there is bug, which also exists in the master branch.

.expect("Component needs required_size implemented in order to be embedded in a popup");

this panic actually can happen, if the popup is open and console area get smaller. (I got this somehow while dragging the console.) I have already fixed this. I prefer to keep the changes as small as possible to get MRs merged quickly, (especially there is an approve now 😄 ) should I add some commits to this MR or better wait until this get merged?

@pascalkuthe
Copy link
Member

@pascalkuthe there is bug, which also exists in the master branch.

.expect("Component needs required_size implemented in order to be embedded in a popup");

this panic actually can happen, if the popup is open and console area get smaller. (I got this somehow while dragging the console.) I have already fixed this. I prefer to keep the changes as small as possible to get MRs merged quickly, (especially there is an approve now 😄 ) should I add some commits to this MR or better wait until this get merged?

can you push them to a seperate branch and link them? It should really be a bug in whatever component is embedded inside the popup rather than popup itself.

@karthago1
Copy link
Contributor Author

done #10507. it shows much changes, because this commit is included there.

Make the popup positions more consistent.
Improvements:
1. if the signature popup content is bigger than the available space,
   then the popup is always shown under the cursor, even if there more
   space above the cursor than below
2. There is no mutation anymore inside required_size. Maybe in the future
   we can update all widgets to have no mutations and change the trait

Signed-off-by: Ben Fekih, Hichem <[email protected]>
to speed up the rendering a little

Signed-off-by: Ben Fekih, Hichem <[email protected]>
@karthago1
Copy link
Contributor Author

as request move the commit 5deff63 to this PR. I didn't modify the other one

@the-mikedavis the-mikedavis merged commit 4b8bcd2 into helix-editor:master Apr 20, 2024
6 checks passed
@pascalkuthe pascalkuthe mentioned this pull request Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants